feat(csc): full-duplex reader-miss coalescing + invalidation batching - #3965
feat(csc): full-duplex reader-miss coalescing + invalidation batching#3965ndyakov wants to merge 31 commits into
Conversation
…nvalidation batching
Builds on the CSC refresh-on-invalidate + base miss-coalescing PR. Adds:
- Coalescing MODES for the reader-miss path, selected by config:
* "workers" (default): a small pinned worker pool fetches reserved misses.
* "fullduplex": one held connection with a writer + reader goroutine pair
pipelines the reserved misses (concurrent, replies streamed back). Ordering
is preserved: the miss-coalescer's reservation dedups concurrent misses of
the same key to a single fetch, and each caller blocks on its own request's
completion — the caching client is blocking per goroutine, so no goroutine
ever sees its own reads reordered; full-duplex only overlaps independent
goroutines' fetches on the wire.
- Invalidation batching: coalesce invalidation-driven cache deletes within a
configurable window instead of one delete per push.
Config (no environment variables): new AutoPipeline-free Options fields —
ClientSideCacheCoalesceMisses (enable), ClientSideCacheCoalesceMode
("workers"/"fullduplex"), ClientSideCacheCoalesceWorkers, and
ClientSideCacheInvalidationBatchWindow. Prototype-only read-path telemetry
(READPATH_LOG + LocalCache stat counters) removed; no stats surface added.
Measured on a 50ms-RTT WAN proxy under invalidation churn, the coalescing fixes
turn published v9.22.0's miss-stampede collapse (p99 up to ~1.1s, throughput
floored) into ~5x throughput at ~1 RTT p99, matching rueidis; the full-duplex
mode gives the tighter mid-range tail at fewer connections. See
AP_CSC_TWOCLIENT_VS_RUEIDIS.md.
Depends on: feature/csc-refresh-and-miss-coalescing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb8681e82b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- lint (the only red CI check): drop the redundant `chan struct{}` type from the
recycle declaration (ST1023) and //nolint:unused the deliberately-off, measured
cscRefreshCooldown knob.
- Reject the "pinned" PROTOTYPE engine from the public ClientSideCacheCoalesceMode
option: it holds a connection with no idle invalidation drain and can serve
stale values (cursor HIGH). It now falls back to "workers" and is reachable
only via an internal benchmark hook (cscForcePinned).
- Pass the new CSC miss-coalescing / invalidation-batching knobs through
UniversalOptions.Simple() (ClientSideCacheRefreshOnInvalidate, CoalesceMisses,
CoalesceMode, CoalesceWorkers, InvalidationBatchWindow) so UniversalClient
users can enable them (codex P2).
- Give the invalidation batcher a stop path: it is stopped and cleared when the
last user releases the binding (releaseLocked), and ensureBatcher refuses to
start one for an already-released binding, so its goroutine no longer lives
past the binding re-arming its timer forever; a later re-acquire starts fresh.
- Full-duplex session: acquire the pool connection only after the first miss
arrives, and release it if CSC serving was disabled meanwhile, so an idle
session no longer starves a small pool (PoolSize:1) until PoolTimeout.
Adds unit tests for the pinned-mode rejection and the UniversalOptions passthrough.
💡 Codex Reviewgo-redis/csc_miss_coalesce_modes.go Lines 362 to 364 in 2f6a9e5 In full-duplex CSC, go-redis/csc_miss_coalesce_modes.go Lines 166 to 168 in 2f6a9e5 With go-redis/csc_miss_coalesce_modes.go Lines 373 to 374 in 2f6a9e5 After a clean full-duplex recycle, ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR extends the client-side caching (CSC) subsystem by moving miss-coalescing and invalidation-batching controls into Options/UniversalOptions, adding a full-duplex miss-coalescing engine, and introducing windowed background batching for invalidation-driven deletes.
Changes:
- Add new CSC configuration knobs to
Optionsand propagate them throughUniversalOptions.Simple(). - Introduce selectable miss-coalescing modes (
workersdefault,fullduplex), including a new full-duplex session engine. - Add optional windowed invalidation batching to offload cache deletes from the push-notification read path.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| universal.go | Adds new CSC config fields to UniversalOptions and propagates them into Options via Simple(). |
| options.go | Adds new CSC config fields to Options and documents coalescing/batching behavior. |
| csc_refresh_support.go | Removes env-var support helpers now that options drive behavior. |
| csc_refresh_on_invalidate.go | Removes env-var gates/knobs and switches refresh publishing to tracked main-pool connections. |
| csc_miss_coalesce.go | Refactors miss coalescer to be options-driven and to support multiple engines with shared helpers/stats. |
| csc_miss_coalesce_modes.go | Adds the pinned prototype and full-duplex miss-coalescing engines and related session lifecycle logic. |
| csc_miss_coalesce_modes_test.go | Adds coverage for full-duplex idle push draining correctness (positive + negative control). |
| csc_inval_batch.go | Introduces the windowed background invalidation batcher implementation. |
| csc_integration.go | Integrates invalidation batching into the push invalidation handler and threads the window from Options. |
| csc_coalesce_options_test.go | Adds tests for public mode selection behavior and UniversalOptions.Simple() propagation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f6a9e55ef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…; doc - Full-duplex readOne now applies the read reply and lets fulfillCached gate only the cache publish on the captured conn id/generation -- matching the workers/pinned engines -- instead of failing the caller with ErrClosed and losing a good reply on a mid-flight id/gen change. - peekAndProcessPushNotifications also drains when the reader has buffered bytes (HasBufferedData), not only when the socket is readable (MaybeHasData), so a buffered invalidation is processed on the idle tick. - Workers-mode doc corrected: a tracked connection is acquired/released per batch, not held across batches.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a5a231afa5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
csc_miss_coalesce_modes.go:14
- The file header labels these engines as "PROTOTYPE", but the fullduplex engine is selectable via the public ClientSideCacheCoalesceMode option and described as feature-complete in this PR. This comment is misleading for maintainers/users reading the code.
// Alternate miss-coalescer engines (PROTOTYPE, benchmark comparison).
csc_miss_coalesce_modes.go:134
- The comment says a clean recycle continues immediately, but the code always waits for cscModeBackoff after every session end. Either skip the backoff on clean recycle, or update the comment so behavior and documentation match.
// Session ended on a connection error or a clean recycle. On error, back
// off briefly so a persistent dial failure does not hot-spin; a clean
// recycle continues immediately.
- A coalesced miss re-runs uncached when CSC serving is disabled after the miss was reserved (RESP3 downgrade / CLIENT TRACKING loss during a conn re-init), via an internal retry-uncached sentinel that processCached catches, instead of surfacing a spurious pool.ErrClosed for a valid cacheable read. - A caller whose context cancels mid-fetch no longer races the coalescer: a CAS interlock (claimAbandon/claimApply) hands the Cmder to exactly one of the caller or the applying worker, so the worker never writes a Cmder the caller has taken back. The reply is still classified and published to the shared cache, and a cancelled Get returns the context error deterministically (matching the non-coalesced path). - A fetch after coalescer shutdown returns pool.ErrClosed instead of hanging on a post-drain enqueue race. Adds TestClassifyCachedReply, TestCSCMissReqClaimInterlock, TestCSCMissCoalesceAbandonedFetchNoRace, TestFullDuplexDisabledMidMissRetriesUncached.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (2)
csc_miss_coalesce_modes.go:216
- sessErr is an atomic.Value that is only Store()'d on I/O failure, but errored()/reasonErr() call Load() unconditionally. atomic.Value panics on Load before the first Store, so the graceful-recycle path can crash the client. Initialize sessErr with a non-nil typed holder (or switch to atomic.Pointer) so Load is always safe.
errored := func() bool { _, ok := sessErr.Load().(error); return ok }
reasonErr := func() error {
if e, ok := sessErr.Load().(error); ok {
return e
}
csc_miss_coalesce.go:25
- The header comment still says miss coalescing is "env-gated", but the gating was moved to Options.ClientSideCacheCoalesceMisses. This is now misleading for readers and users.
// Reader-miss coalescing (PROTOTYPE, env-gated).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cae5e11b8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…ange - A full cache flush (FLUSHDB/FLUSHALL) cleared the cache but left the invalidation batcher's queued per-key deletes, which then fired and evicted entries repopulated after the flush (an extra miss within the window). Drop the batcher's pending queue on flush. - A running batcher's window is fixed at creation, so a second client binding to the same shared handler with a stricter window kept the old cadence and its staleness bound did not hold. setInvalBatchWindow now drops the running batcher on a window change so the next invalidation starts a fresh one with the new window.
setup-go's "1.26.x" resolved to go1.26.5, which govulncheck flags for two standard-library vulnerabilities fixed in go1.26.6: GO-2026-6090 (crypto/tls) and GO-2026-5972 (encoding/asn1). Track the latest stable toolchain so future security patches are picked up automatically instead of pinning a patch.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
csc_inval_batch.go:60
- The buffered
dropChsignal does not order ahead of the timer case and cannot stop anapplyalready in progress. Around a window boundary,runcan selectt.Cand apply the pre-flush batch aftercache.Flush()(and after a reader repopulates the key), evicting the fresh entry despite this method's guarantee. Coordinate flush/drop with an epoch or mutex and wait for/neutralize any in-progress apply so all pre-FLUSH deletes are definitively superseded.
// Signal run() to clear its in-progress batch; the cap-1 buffer means a signal
// is never lost even if run() is not currently selecting.
select {
case b.dropCh <- struct{}{}:
default:
csc_miss_coalesce.go:336
getConncallsLimiter.Allowonly once for the whole batch, while every request inbatchis a separate client operation. This lets batches bypass per-operation rate/circuit limits and reports only one aggregate result, contrary to theLimitercontract inoptions.go:37-45. Check and report the limiter per request, excluding denied requests from the wire batch; use_getConnfor the underlying batch connection so it does not consume an extra limiter operation.
cn, err := c.getConn(ctx)
csc_miss_coalesce_modes.go:168
- This acquires one limiter permit via
getConnand holds it for the entire full-duplex session, which can execute thousands of independent operations for up to 30 seconds. A concurrency limiter may therefore reject unrelated commands while this permit sits idle, and a rate/circuit limiter never sees individual miss results. ApplyAllow/ReportResultper request and acquire the session connection through_getConninstead.
getCtx, getCancel := opCtx()
cn, err := c.getConn(getCtx)
getCancel()
csc_miss_coalesce_modes.go:158
- Waiting for the first miss does not prevent idle starvation after that miss completes: the session keeps the connection until the 30-second recycle timer. With the valid
PoolSize: 1configuration, a miss succeeds but the caller's next non-cacheable command (for exampleSETorPING) cannot acquire the sole pool turn and times out. Return the session connection once its in-flight queue drains and no miss is queued, or reject/fallback from full-duplex mode when the pool cannot reserve another connection.
// Do not hold a pool connection while idle: wait for the first miss BEFORE
// acquiring. An eagerly-held session connection would, at a small pool
// (PoolSize:1), starve non-cacheable commands (PING/SET/uncached reads) until
// PoolTimeout while the session sat waiting for work. The pulled miss is
// written first by the writer below.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 636ca531e9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- The invalidation batcher's stop path drains keys still buffered in its channel into the final flush, so a window-change rebuild loses no queued deletes (they would otherwise serve pre-invalidation values until TTL/MaxStaleness). Adds TestInvalBatchStopAppliesQueuedDeletes. - ensureBatcher reads the window under the handler lock (not the caller's pre-lock snapshot) and returns nil at window 0, so a concurrent tighten or disable cannot be undone by a batcher rebuilt with the stale cadence. - setInvalBatchWindow folds windows strictest-wins across attached clients (explicit 0/inline strictest, then smaller nonzero): a later, looser attach can no longer lengthen batching past an earlier client's staleness bound. The effective window resets when the last user releases. - The miss-coalescer flush budget derives from the configured WriteTimeout + ReadTimeout (plus a 5s pool-Get floor) instead of a fixed 5s, so clients with deliberately long timeouts do not see only coalesced misses clipped early.
- The stop/recycle drain backstop force-closed the conn after one fixed batch budget, which could cut a HEALTHY drain of a deep in-flight pipeline (up to cscFullDuplexDepth replies, ~in-flight x RTT, possibly under maintenance-relaxed timeouts). The supervisor now samples the in-flight queue per budget interval: any consumed reply extends the wait; only a zero-progress interval closes the conn. - TestReleaseConnRemovesConnectionAfterPartialPushRead skips the benign case where the drain probe times out before consuming any byte: the frame is intact and re-pooling is safe; only a Put after bytes moved into the reader is the desync bug the test pins.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 883be5612e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Removing the SetDeadline reset left a prior WithReader's read deadline armed; once expired, rawConn.Read fails fast with i/o timeout before the non-blocking peek runs, the CSC drainer treats the error as fatal and removes an idle-but-healthy conn, evicting its cache coverage. The full-duplex coalescer concentrates coverage on one held conn, so one spurious removal wiped the whole cache (WAN: hit rate 25%, throughput cut to a third). Clear only the READ deadline: the write deadline stays untouched because checkForData runs concurrently with command writes on a held full-duplex connection - which is what the old full SetDeadline reset clobbered. Regression test pins the expired-deadline case.
- The abandon interlock gated only the reply side, so the session writer could serialize an abandoned caller's cmd args - a use-after-return on mutable args (e.g. a []byte key), with the reply publishable under the original cache key. The writer now claims each request (PENDING->WRITING) around arg serialization, releases before the flush, and drops abandoned requests with their reservation cancelled; abandonOrWait yields through a WRITING claim, which spans one in-memory batch encode. - The drain backstop measures progress as completed reads (readsDone) instead of len(inflight), which was blind to the reader's active read, and its interval honors the connection's effective (maintenance-relaxed) read timeout via pool.Conn.EffectiveReadTimeout so a legitimately deep read is not cut short. - The partial-push-read test completes the frame and requires a whole- frame parse before skipping: an empty reader buffer alone does not prove the probe consumed nothing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c7c6fc1d7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Replace the WRITING ownership claim with a wire snapshot taken in fetch, while the caller still owns cmd: the session writer writes only engine-owned bytes and never reads cmd, so no ownership window spans arg serialization (which could implicitly flush and block on the socket), abandoning callers return immediately, and a post-abandon arg mutation can neither reach the wire nor publish under the original cache key. Abandoned fetches complete again, so their reservations settle instead of stranding IN_PROGRESS behind the miss backlog. Also: fetch's Close-race branch drains the queue after cancelling (a request landing after the shutdown drain is not retained via a live WithTimeout clone), and the recycle backstop honors explicitly disabled read deadlines - it waits for the session instead of force- closing on a zero-progress interval, escalating only once stop fires so Close still terminates.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e844cc0425
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- timedPushDrain reads under a hard deadline: WithReader let an active maintenance-relaxed timeout replace the 1ms probe cap, parking the FD session reader for the relaxed duration on a no-data probe while it held the connection. WithReaderHardDeadline bypasses relaxation and clears the deadline on exit (the negative-ReadTimeout cleanup block is now redundant and removed). - The supervisor also watches the recycle channel, so a reader-triggered recycle (handoff/close-on-put) gets the same bounded progress-based drain backstop as the age-triggered path - a stalled in-flight reply can no longer postpone a requested handoff until Close. - The deadline-less recycle carve-out tests rt <= 0: Options.init normalizes ReadTimeout -1 (indefinite) to 0, so the indefinite mode was still force-closed after one budget interval.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb220c9dd9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Queued invalidations carry the batcher epoch they were enqueued under; drop() bumps the epoch instead of draining, and apply skips stale-epoch items (queue and in-progress batch alike, so the dropCh signal is gone). The flush handler bumps before cache.Flush(), so a per-key invalidation racing in from another tracked connection after the flush keeps its delete - the drain-based drop could discard it and leave a repopulated entry stale. Dedup is per key+epoch so a post-flush re-arrival is not swallowed by the duplicate check. - stopCSCRefresher clears the shared handler's refresh binding only while it still points at the closing client's own queue: a sibling client sharing the cache/processor keeps its refresher fed. - Teardown deactivates cscActive before stopping the coalescer, and fetch's stop paths settle with errCSCRetryUncached so a clone's miss racing the teardown window re-runs uncached instead of failing with ErrClosed on an open pool; the mid-apply branch returns the settle result itself rather than discarding a successfully applied reply.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8771aa7fd1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- apply/drop are serialized by a batcher mutex: apply snapshotting the epoch once let a concurrent FLUSH land mid-batch and stale-epoch deletes still run post-flush, evicting repopulations - the case the epoch exists to prevent. drop() holding the mutex means an in-flight batch finishes before the flush (harmless: the flush wipes it) and every later apply sees the new epoch. - Both FD-session push paths (the reader's per-reply drain and timedPushDrain) hand handlers the nonblocking cscHandlerClient adapter like the background drainer does: a custom push handler calling Close() signaled-and-deferred instead of deadlocking on mc.wg.Wait while the session reader is parked inside that handler.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 911bf54c28
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- The shutdown drain and the GC cleanup settle queued misses with errCSCRetryUncached, matching fetch's stop paths: a caller woken during the teardown window re-runs its read on the still-open pool instead of surfacing ErrClosed. - The shared handler tracks refresh bindings in a stack: clearing the ACTIVE binding restores the next-newest live sibling instead of nil, so closing the newest owner no longer severs an older client's still-running refresher (the identity check alone only protected the reverse order). - The push-handler adapter closes through the canonical *Client via a weak back-pointer: Client.Close also stops the cached autopipeliners, which baseClient.Close bypassed, leaving flush goroutines running against closed pools. Weak so the wrapper stays collectible and the drop-without-Close cleanup still fires.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37a130c484
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- The drain backstop honors the write side too: the interval covers the connection's effective write timeout (pool.Conn.EffectiveWriteTimeout) and the deadline-less carve-out fires when either read or write deadlines are disabled - a deadline-free write blocked flushing a large request shows no read progress by construction and must not be cut by an age/handoff recycle. - The opaque-transport speculative probe runs only with the built-in push processor: a custom processor may surface the empty-probe timeout per its contract, which would remove and redial a healthy session on every idle probe. - withTimeout clones whose timeouts diverge from the owner's bypass miss coalescing: the shared engine reads the owner's options, so a coalesced miss would honor the owner's deadlines, defeating WithTimeout. Hits and uncached fetch caching are unchanged. - releaseLocked clears refresh/refreshStack with the binding: a client dropped without Close must not leave a dead queue for a successor to inherit or restore. - The drop-without-Close cleanup drain settles with errCSCRetryUncached like every other stop path (the round-22 hunk for this block was a silent patch no-op; applied for real).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d97d831eb7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Capping the recycle age at the raw ConnMaxLifetime before consulting ExpiresAt collapsed every positive-jitter connection back to the unjittered lifetime, re-synchronizing session recycles across clients started together - the herd ConnMaxLifetimeJitter exists to prevent. Bound the age by the connection's actual absolute expiry only (jitter included), subject to the independent 30s session cap.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95ea487eb0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Log-and-continue past a surfaced push-processor error let the reader proceed into ReadRawReply on a possibly desynchronized stream: a processor that consumed part of a push before failing leaves the next bytes mid-frame, and a fragment could be applied as a caller's reply and published to the cache. Treat the error as session-fatal (close and remove the connection), matching drainPushNotifications - the built-in processor surfaces only mid-frame failures, and a custom processor's contract cannot prove no bytes were consumed.
A coalesced miss returned from processCached without touching processState, so the native recorder saw zero attempts and a nil connection (empty server.address), and engine-failed requests bypassed the error callback that processWithRetry emits. fetch now returns the serving session connection (recorded by the reader before the done settle, which is the happens-before edge) and processCached stamps it into processState as one attempt; applyAndSettle and settleErr emit the native error callback with parity to processWithRetry and the FD autopipeline engine's failReqs - the retry-uncached sentinel is excluded, since its command re-runs on the fully instrumented path. Refs the deferred-observability thread on #3965.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ce21ffb50f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if werr != nil { | ||
| fail(werr) | ||
| mc.settleAllErr(buf, 0, werr) |
There was a problem hiding this comment.
Attribute write failures to the held connection
When WithWriter fails after the session has acquired cn—for example on a write timeout or socket error—this settles every request before assigning req.servedBy. Consequently processCached still records zero attempts and a nil connection, and settleErr invokes the error metric callback with a nil connection even though these commands reached a specific session connection. Fresh evidence beyond the earlier telemetry thread is that only the reply-reader paths assign servedBy; assign the session connection before attempting/settling the write batch so write-side failures retain the same attribution.
Useful? React with 👍 / 👎.
| if e := c.peekAndProcessPushNotifications(sctx, cn); e != nil { | ||
| fail(e) | ||
| return |
There was a problem hiding this comment.
Allow fragmented pushes to finish on held sessions
When a push notification arrives in fragments more than 1 ms apart—particularly over TLS, where MaybeHasData only proves that some ciphertext is readable—this idle-session call routes the frame through timedPushDrain and its 1 ms hard deadline. If the processor consumes the available prefix before the remainder arrives, the timeout is treated as fatal and the healthy session connection is closed, failing in-flight misses; the background drainer already uses cscDrainHardReadCap specifically to tolerate this fragmented-frame case. Use the longer fragmented-frame budget after readiness is established, reserving the 1 ms deadline for speculative no-data probes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ce21ffb. Configure here.
| if cl := c.cscClientWeak.Value(); cl != nil { | ||
| return cl.Close() | ||
| } | ||
| return c.baseClient.Close() |
There was a problem hiding this comment.
Clone Close bypasses owner teardown
Medium Severity
closeCanonical is meant to tear down through Client.Close, but cscClientWeak is only set on the owner in cscRegisterCleanups and is not wired on WithTimeout clones. Those clones also omit cscDrainHandle, so a push handler calling Close on the cscHandlerClient installed by timedPushDrain falls through to baseClient.Close, which closes the shared pools without owner CSC/autopipeliner teardown via cscLifecycleOwner.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit ce21ffb. Configure here.


Full-duplex reader-miss coalescing + invalidation batching
Builds on the CSC refresh-on-invalidate + base reader-miss coalescing PR
(
feature/csc-refresh-and-miss-coalescing). Two additions, both config-driven.1. Full-duplex miss coalescing (reader-miss path)
Concurrent cache misses of the same key are already deduped to a single fetch
via a per-key reservation (one owner fetches; the rest block on it and share the
result). This PR adds the engine that dispatches the owners' fetches: a held
tracked connection with a writer + reader goroutine pair that pipelines
reserved misses — commands stream out while replies stream back.
Misses are caller-blocking (a real request waits on every fetch), so the
engine is latency-first:
what is already queued), never waited-for;
no batch phase-lock, no pool
Geton the hot path;next miss, so an idle coalescer holds zero connections;
held connection even while idle, and returns the connection to the pool for
handoff/lifetime/pool-hook processing (idle grace, recycle age bounded by the
connection's remaining
ConnMaxLifetime,ShouldHandoffchecks).Ordering is preserved. Each caller blocks on its own request's completion,
so a goroutine never issues its next command until this value is in hand — the
engine only overlaps independent callers' fetches on the wire.
Earlier engine variants ("workers": N pooled connections with half-duplex
batches; "pinned": a benchmark prototype) were removed during review: the
per-batch round trip added tail latency to exactly the path where a caller is
waiting, and the batching advantage is preserved by the writer's opportunistic
packing. One engine, no tuning surface.
2. Invalidation batching
Coalesce invalidation-driven cache deletes within a configurable window instead
of one delete per push frame — smooths bursty invalidation churn. Background
traffic is batching-first by design (nobody waits on it): windowed, deduped,
with the delete queue preserved across batcher rebuilds and dropped on
FLUSHDB/FLUSHALL(a full flush supersedes queued per-key deletes).Config (no environment variables)
New
Optionsfields (flatClientSideCache*, matchingClientSideCacheRefreshOnInvalidate), mirrored inUniversalOptions:ClientSideCacheCoalesceMisses bool— enables the coalescer (requires thebuilt-in
LocalCache; ignored for customCacheimplementations).ClientSideCacheInvalidationBatchWindow time.Duration— 0 (default) appliesinvalidations inline; a nonzero window batches them (set it no larger than the
cache
MaxStaleness). Shared-handler clients fold windows strictest-wins.Observability rides the client's normal telemetry: coalesced misses fire the
otel operation-duration and error callbacks like any other command path (no
separate stats API — the engine's internal counters are test-only).
Hardening (from review)
The review rounds hardened the engine's lifecycle and edge behavior, including:
single-flight token settlement on every path (no caller ever hangs, no
reservation leaks); a CAS ownership interlock so a ctx-cancelled caller's
Cmderis never written concurrently;
Closeinterruption of blocked acquisitions andsocket reads (bounded drain, then conn close); retry-uncached fallback when CSC
is disabled mid-miss; GC cleanup for clients dropped without
Close;WithTimeoutclones sharing the coalescer; and held-connection probe safety(no deadline clobbering of concurrent I/O; opaque-transport idle-drain fallback).
Note
High Risk
Large changes to CSC correctness paths (coalescing, invalidation timing, connection lifecycle, and stale-serving edge cases) on the hot read path; misbehavior would affect cache freshness and client shutdown semantics.
Overview
Adds config-driven client-side caching for miss coalescing and invalidation batching:
ClientSideCacheCoalesceMisses,ClientSideCacheInvalidationBatchWindow, and related knobs onOptionsandUniversalOptions(replacing env-gated prototypes).UniversalOptions.Simple()now copies them so universal clients can enable the features.Full-duplex miss coalescing pipelines reserved cache misses on a held tracked connection (writer + reader), with immediate writes for lone misses, idle session release, push draining on the held conn, CAS-guarded
Cmderownership, wire snapshots at enqueue,errCSCRetryUncachedwhen CSC stops mid-miss, GC cleanup for clients dropped withoutClose, and OTel attribution on the serving connection.WithTimeoutclones skip shared coalescing when timeouts differ from the owner.Invalidation batching (
csc_inval_batch.go) defersLocalCachedeletes to a background goroutine when the batch window is nonzero, with strictest-window wins across shared handlers, epoch-based supersede on full flush, and safe stop/inline-delete fallbacks.Shared invalidate handler gains refresh binding stacks (siblings survive close), batcher lifecycle tied to attach/release, and integration at CSC attach. Refresh refetches use the main tracked pool instead of the untracked pipeline pool. Pool readiness fixes (
SetReadDeadlineclear,EffectiveReadTimeout, buffered-data push drain) support held-connection paths. govulncheck uses Gostableinstead of1.26.x.Reviewed by Cursor Bugbot for commit ce21ffb. Bugbot is set up for automated code reviews on this repo. Configure here.